Using GenerateName is what OLMv0 does, correct? I think it does this so that multiple validating/mutating webhook configuration objects can be created when own/single namespace bundles are installed multiple times.

That is an anti-goal for OLMv1, so perhaps it makes sense to deviate from OLMv0's use of GenerateName in this case.

Unless there is a really strong reason not to, I'd prefer to use a deterministic name here instead, as that would ultimately make the set of on-cluster objects deterministic as well. I'm sure there are many benefits, but one that comes to mind is that ClusterExtension SAs would be able to specify the deterministic name in advance when setting up RBAC for get/update/patch/delete verbs with the correct resourceName.

Another benefit: using a deterministic name here means that this webhook can only be installed once, regardless of the install modes supported by the extension. That is directly in line with one of the stated invariants of OLMv1 (no two cluster extensions can manage the same object)

I've changed it to use Name instead. I'm trying to think if there's foot gun potential. I might be a little more defensive here and ensure that any - suffix is removed from what is defined in the CSV to avoid any bad name errors.

We might already, but I'll double check and ensure that all webhook names defined in the CSV (for a particular type of webhook) are unique.

I guess if there is a naming clash, i.e. two different bundles call their webhook "my-webhook", the user can just install the bundle in another namespace as an escape hatch...wdyt?

So, now I understand better the @joelanford recommendation/idea: https://redhat-internal.slack.com/archives/C0881N26CGP/p1745960594662879?thread_ts=1745942335.360109&cid=C0881N26CGP

I am OK with since in OLMv1 we should have one instance of each project only the cluster

👍

A client.Object is not guaranteed to have a populated GVK. If we need guarantees about setting the GVK on the unstructured type, we should also pass in a scheme so that we can lookup the GVK based on the object type.

See https://github.com/kubernetes-sigs/controller-runtime/blob/6ad5c1dd4418489606d19dfb87bf38905b440561/pkg/client/apiutil/apimachinery.go#L95

For now, I've just hardened it by checking if version and kind are empty. I don't think we need to add the complexity of passing a scheme. At least not for now =D

If we want the GKV and use it is the right approach then, I think we should use the controller-runtime implementation to do so ( GVKForObject ): https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/apiutil/apimachinery.go#L95C6-L95C18

I don't think we need it right now. The precondition for the function is that the client.Object will carry the GVK. If not, then error. But, I'd say check how we're using it in this PR and let me know if that's not the right way to create an object with GVK.

Do you think that's acceptable since we only use it with CertManager? That way, we'll have the GKV. Is that what you were thinking?

I just don't get what it buys us apart from allowing the function to take client.Objects that don't have a gvk set.

I guess my question is, if we do:

ToUnstructured(&certmanagerv1.Certificate{
		TypeMeta: metav1.TypeMeta{
			APIVersion: certmanagerv1.SchemeGroupVersion.String(),
			Kind:       "Certificate",
		}
		...
)

do we get anything out of adding the scheme? Or would adding it just enable us to do something like:

ToUnstructured(&certmanagerv1.Certificate{}, scheme)

or take arbitrary client.Objects at the cost of creating a new scheme and registering the certmanagerv1 scheme to it?

Since we don't have a need for handling arbitrary client.Objects, is there any point in making the method more complex right now?

If we want to add support for new providers later, how do you envision that looking from a feature gate standpoint?

From what I understand, downstream will use a different provider, but the implementation remains the same.

I don’t see a strong need to support multiple providers upstream for now — downstream will likely stick with one as well. Since this is under an alpha flag, we have flexibility to adjust later.

So I think this is fine as-is and no need to optimize prematurely. We can revisit if we decide to support more providers in the future.

Hi @perdasilva

we would need to do more checks than that:

• The name cannot have more than : 253-character limit
• The name need to be a valid DNS name;

We can use apimachinery to verify if is valid with: https://pkg.go.dev/k8s.io/apimachinery/pkg/util/validation#IsDNS1123Subdomain

https://github.com/kubernetes/apimachinery/blob/954960919938450fb6d06065f4bf92855dda73fd/pkg/util/validation/validation.go#L216-L229

So, it seems to be missing in the validator.
Could we please add this check?

I can add it. But, I think a better approach would be (if it's possible) to validate the csv against its openapi schema. But I don't know that there's a simple way to do that =S

what did we do in operator-sdk? Do we have any bundle validation code/library we can rip out from that?

I can add it. But, I think a better approach would be (if it's possible) to validate the csv against its openapi schema. But I don't know that there's a simple way to do that =S

That is definitely a nice idea. 👍

We’d need to try using the markers for validation: https://book.kubebuilder.io/reference/markers/crd-validation
Add the annotations and re-generate the CRDs where the types are defined.

I think it would be something like

// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:MinLength=1

what did we do in operator-sdk? Do we have any bundle validation code/library we can rip out from that?

I think we should consider checking this at runtime, since currently it isn’t validated. But, maybe it's a bit of an overstatement, though—if the name is invalid, the same check used by k8s.io/apimachinery/pkg/util/validation (IsDNS1123Subdomain) will be enforced by Kubernetes itself.

In the past, we used IsDNS1123Subdomain for scaffolding in the SDK (see: https://github.com/operator-framework/operator-sdk/pull/953/files). The SDK uses Kubebuilder, and in Kubebuilder we also rely on the same validation logic (we just copied it to avoid adding a dependency). See; https://github.com/search?q=repo%3Akubernetes-sigs%2Fkubebuilder%20IsDNS1123Subdomain&type=code

If we decide to add the check then, IMO the best approach is to use the checks from k8s.io/apimachinery/pkg/util/validation instead any internal lib.

I've added the validation functions that use the IsDNS1123Subdomain under the hood. I'm also cooking up a scheme where we generate a validator from based on an imported ClusterServiceVersion CRD. This will give us the ultimate security that the CSV is valid (though additional validators around referential integrity, uniqueness, etc. will still be required)

But this validator will come in a follow up PR

Could we just add an e2e test before merging, or at plan to do so in a follow-up?

We might be able to reuse something from here:
https://github.com/search?q=repo%3Ak8s-operatorhub%2Fcommunity-operators%20generateName&type=code
(Since we’re using real operators) — this could help validate that the webhooks are working correctly.

lets do this as a follow up since we have some work we need to do before we can enable e2es for FGs